Can't select an empty report from the Reports > Reports page #81036
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
melvin-bot
bot
requested review from
trjExpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const emptyReports = useMemo(() => { | ||
| if (type === CONST.SEARCH.DATA_TYPES.EXPENSE_REPORT && isTransactionGroupListItemArray(data)) { | ||
| return data.filter((item) => item.transactions.length === 0); | ||
| } |
There was a problem hiding this comment.
❌ PERF-13 (docs)
The isTransactionGroupListItemArray(data) type guard is called inside both the emptyReports and selectedItemsLength useMemo blocks. This type check does not depend on the iterator and produces the same result on every iteration.
Suggested fix: Hoist the type check outside the reduce/filter operations:
const emptyReports = useMemo(() => {
if (type !== CONST.SEARCH.DATA_TYPES.EXPENSE_REPORT) {
return [];
}
if (!isTransactionGroupListItemArray(data)) {
return [];
}
return data.filter((item) => item.transactions.length === 0);
}, [data, type]);
const selectedItemsLength = useMemo(() => {
const selectedTransactionsCount = flattenedItems.reduce((acc, item) => {
const isTransactionSelected = !!(item?.keyForList && selectedTransactions[item.keyForList]?.isSelected);
return acc + (isTransactionSelected ? 1 : 0);
}, 0);
const isExpenseReportType = type === CONST.SEARCH.DATA_TYPES.EXPENSE_REPORT;
const isGroupedData = isTransactionGroupListItemArray(data);
if (isExpenseReportType && isGroupedData) {
const selectedEmptyReports = emptyReports.reduce((acc, item) => {
const isEmptyReportSelected = !!(item.keyForList && selectedTransactions[item.keyForList]?.isSelected);
return acc + (isEmptyReportSelected ? 1 : 0);
}, 0);
return selectedEmptyReports + selectedTransactionsCount;
}
return selectedTransactionsCount;
}, [flattenedItems, type, data, emptyReports, selectedTransactions]);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| return selectedTransactionsCount; | ||
| }, [flattenedItems, type, data, emptyReports, selectedTransactions]); | ||
|
|
||
| const totalItems = useMemo(() => { |
There was a problem hiding this comment.
❌ PERF-13 (docs)
The isTransactionGroupListItemArray(data) type guard is called inside the totalItems useMemo and does not depend on iteration. Additionally, the type check and filtering logic are duplicated for both branches.
Suggested fix: Hoist the type check and consolidate the filtering logic:
const totalItems = useMemo(() => {
const isExpenseReportType = type === CONST.SEARCH.DATA_TYPES.EXPENSE_REPORT;
const isGroupedData = isTransactionGroupListItemArray(data);
const selectableTransactions = flattenedItems.filter((item) => {
if ('pendingAction' in item) {
return item.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE;
}
return true;
});
if (isExpenseReportType && isGroupedData) {
const selectableEmptyReports = emptyReports.filter((item) =>
item.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE
);
return selectableEmptyReports.length + selectableTransactions.length;
}
return selectableTransactions.length;
}, [data, type, flattenedItems, emptyReports]);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| } | ||
| const areItemsGrouped = !!validGroupBy || type === CONST.SEARCH.DATA_TYPES.EXPENSE_REPORT; | ||
| const totalSelectableItemsCount = areItemsGrouped | ||
| ? (filteredData as TransactionGroupListItemType[]).reduce((count, item) => { |
There was a problem hiding this comment.
❌ PERF-13 (docs)
The isTransactionReportGroupListItemType(item) type guard is called on every iteration inside the .reduce() callback. This type check does not depend on the iterator variable and produces the same boolean result for a given item.
Suggested fix: Perform the check once before filtering:
const totalSelectableItemsCount = areItemsGrouped
? (filteredData as TransactionGroupListItemType[]).reduce((count, item) => {
const isEmpty = item.transactions.length === 0;
// For empty reports, count the report itself as a selectable item
if (isEmpty && isTransactionReportGroupListItemType(item)) {
if (item.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) {
return count;
}
return count + 1;
}
// For regular reports, count all transactions except pending delete ones
const selectableTransactions = item.transactions.filter((transaction) => !isTransactionPendingDelete(transaction));
return count + selectableTransactions.length;
}, 0)
: filteredData.length;Note: This assumes all items in the filtered data have the same type. If items have mixed types, the current approach is correct and this comment can be disregarded.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
|
||
| setSelectedTransactions( | ||
| Object.fromEntries( | ||
| const allSelections: Array<[string, SelectedTransactionInfo]> = (filteredData as TransactionGroupListItemType[]).flatMap((item) => { |
There was a problem hiding this comment.
❌ PERF-13 (docs)
The isTransactionReportGroupListItemType(item) type guard is called inside the .flatMap() callback on every iteration. This type check does not depend on the current iteration and could be optimized.
Suggested fix: While the type check is applied per item (which is appropriate), consider batching the logic to avoid repeated type checks:
const allSelections: Array<[string, SelectedTransactionInfo]> = (filteredData as TransactionGroupListItemType[]).flatMap((item) => {
const isEmpty = item.transactions.length === 0;
const isReportType = isTransactionReportGroupListItemType(item);
if (isEmpty && isReportType && item.keyForList) {
if (item.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) {
return [];
}
return [mapEmptyReportToSelectedEntry(item)];
}
return item.transactions
.filter((t) => !isTransactionPendingDelete(t))
.map((transactionItem) => {
const itemTransaction = transactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionItem.transactionID}`] as OnyxEntry<Transaction>;
const originalItemTransaction = transactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${itemTransaction?.comment?.originalTransactionID}`];
return mapTransactionItemToSelectedEntry(transactionItem, itemTransaction, originalItemTransaction, email ?? '', accountID, outstandingReportsByPolicyID);
});
});Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
|
||
| const currentTransactions = itemTransactions ?? item.transactions; | ||
|
|
||
| // Handle empty reports - treat the report itself as selectable |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The pattern item.transactions.length === 0 && isTransactionReportGroupListItemType(item) is duplicated throughout this file (lines 572, 762, 817, 1245) and in SearchList/index.tsx. This logic should be extracted into a reusable helper function.
Suggested fix: Create a helper function:
function isEmptyExpenseReport(item: unknown): item is TransactionReportGroupListItemType {
return (
isTransactionReportGroupListItemType(item) &&
item.transactions.length === 0
);
}Then use it consistently:
// Instead of:
if (currentTransactions.length === 0 && isTransactionReportGroupListItemType(item)) {
// Use:
if (isEmptyExpenseReport(item)) {This consolidates the logic, improves maintainability, and makes the intent clearer.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| itemWithSelection = {...item, isSelected: false}; | ||
| } else { | ||
| const hasAnySelected = item.transactions.some((t) => t.keyForList && selectedTransactions[t.keyForList]?.isSelected); | ||
| const isEmptyReportSelected = |
There was a problem hiding this comment.
❌ PERF-13 (docs)
The isTransactionReportGroupListItemType(item) type guard is called inside the .map() callback for every item. This type check could be hoisted outside the conditional check.
Suggested fix: Calculate the type check once per item to improve clarity:
const itemsWithSelection = useMemo(() => {
return data.map((item) => {
let isSelected = false;
let itemWithSelection: SearchListItem = item;
if ('transactions' in item && item.transactions) {
if (\!canSelectMultiple) {
itemWithSelection = {...item, isSelected: false};
} else {
const isEmpty = item.transactions.length === 0;
const isReportType = isTransactionReportGroupListItemType(item);
const isEmptyReportSelected = isEmpty && isReportType && \!\!(item.keyForList && selectedTransactions[item.keyForList]?.isSelected);
const hasAnySelected = item.transactions.some((t) => t.keyForList && selectedTransactions[t.keyForList]?.isSelected) || isEmptyReportSelected;
if (\!hasAnySelected) {
itemWithSelection = {...item, isSelected: false};
} else if (isEmptyReportSelected) {
isSelected = true;
itemWithSelection = {...item, isSelected};
} else {
// ... rest of the logic
}
}
} else {
isSelected = \!\!(canSelectMultiple && item.keyForList && selectedTransactions[item.keyForList]?.isSelected);
itemWithSelection = {...item, isSelected};
}
return {originalItem: item, itemWithSelection, isSelected};
});
}, [data, canSelectMultiple, selectedTransactions]);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e98d167bfe
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for (const reportID of reportIDList) { | ||
| deleteAppReport(reportID, currentUserEmailParam, currentUserAccountIDParam, reportTransactions, transactionsViolations, bankAccountList); |
There was a problem hiding this comment.
When bulk-deleting from the Search page, bulkDeleteReports calls deleteAppReport without the search hash, so the new snapshot update path in deleteAppReport never runs. This means empty reports deleted via bulk actions won’t be marked pendingAction: DELETE in the search snapshot and can remain visible until a refresh or new search. You already pass currentSearchHash when deleting from report view, so this bulk path should do the same to keep the search results consistent.
Useful? React with 👍 / 👎.
| !isOffline && | ||
| selectedTransactionsKeys.every((id) => { | ||
| const transaction = currentSearchResults?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${id}`] ?? selectedTransactions[id]?.transaction; | ||
| if (!transaction) { | ||
| return false; | ||
| } | ||
| const parentReportID = transaction.reportID; | ||
| const parentReport = currentSearchResults?.[`${ONYXKEYS.COLLECTION.REPORT}${parentReportID}`] ?? selectedTransactions[id].report; | ||
| const reportActions = currentSearchResults?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${parentReportID}`]; | ||
| const parentReportAction = | ||
| Object.values(reportActions ?? {}).find((action) => (isMoneyRequestAction(action) ? getOriginalMessage(action)?.IOUTransactionID : undefined) === id) ?? | ||
| selectedTransactions[id].reportAction; | ||
|
|
||
| return canDeleteMoneyRequestReport(parentReport, [transaction], parentReportAction ? [parentReportAction] : []); | ||
| }) | ||
| (selectedReports.length && searchDataType !== CONST.SEARCH.DATA_TYPES.EXPENSE | ||
| ? selectedReports.every((selectedReport) => { |
There was a problem hiding this comment.
In the Reports search, if a user selects at least one full report and also selects individual transactions from other reports, the delete option eligibility now checks only selectedReports and ignores selectedTransactions. That can surface the delete action even when some selected transactions are not deletable (e.g., not owned or not in an open/processing report), leading to a delete attempt that partially fails. The pre-change behavior validated every selected transaction; this new branch should also incorporate any non-report selections when present.
Useful? React with 👍 / 👎.
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
Yes, people should be able to select empty reports. 👍
2 participants
Explanation of Change
Fixed Issues
$ #70591
PROPOSAL: #70591 (comment)
Tests
Offline tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.mov
ANDROID.mov
Android: mWeb Chrome
website.android.mov
ANDROID.WEBSITE.mov
iOS: Native
ios.mov
IOS.mov
iOS: mWeb Safari
website.ios.mov
IOS.WEBSITE.mov
MacOS: Chrome / Safari
website.mov
MacOS: Desktop
desktop.mov